Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Initial libstd support #17

Merged
merged 2 commits into from
Aug 6, 2023
Merged

Initial libstd support #17

merged 2 commits into from
Aug 6, 2023

Conversation

xobs
Copy link
Contributor

@xobs xobs commented Aug 5, 2023

This PR contains two patches required to allow unwinding to be used with libstd.

As background, I'm porting libstd to the Xous operating system. This is an embedded target.

In the past I've applied patches to libunwind in order to get it to work, however those patches are very difficult to get upstream. A recent change in llvm made C compiling difficult (https://reviews.llvm.org/D156642), and additional patches will be needed on top of Rust's hacked-up version of LLVM that adds support for SGX.

With these patches, along with an unwinding backend for the Rust unwind crate, I'm able to run tests and catch panics in Xous.

This patchset does two things:

  1. Add support for making core optional, which is necessary for building as part of the tree
  2. Make text_base optional, which is required due to the fact that Xous doesn't have this information and no platforms currently use it anyway

While it's true that (2) is an API-level change, this API is not documented in either docs.rs or the examples/ directory, so I'm not sure how widely used it is.

@nbdd0121
Copy link
Owner

nbdd0121 commented Aug 5, 2023

I believe the base address can be zero. Probably just using Option<usize> is fine?

@xobs
Copy link
Contributor Author

xobs commented Aug 5, 2023

Good point -- I've adjusted the API accordingly.

@nbdd0121
Copy link
Owner

nbdd0121 commented Aug 5, 2023

CI is failing due to rust-lang/rust#108955 denying lang_items feature and rust-lang/rust#112849 changing the panic display output. Fix in progress...

nbdd0121
nbdd0121 requested changes Aug 5, 2023
View reviewed changes
Copy link
Owner

@nbdd0121 nbdd0121 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is now fixed, please rebase on latest trunk.

src/unwinder/find_fde/custom.rs Outdated
if let Some(text_base) = text_base {
bases = bases.set_text(text_base as _);
}
let eh_frame_hdr =
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my machine this change is undone by running cargo fmt. Did you have a global rustfmt config?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have it as a subdirectory of the rust project, and I think they use some nonstandard settings that rustfmt must have picked up even though it's a different repository.

I've moved it out of the rust directory and re-run cargo fmt without anything in any directories above it.

@nbdd0121
Copy link
Owner

nbdd0121 commented Aug 5, 2023

On the implementation side; do you think it makes sense to just use 0 as text_base when it's unknown instead of None? If textrel is not actually used, then setting it to 0 wouldn't actually cause any problem? I am a little bit reluctant to make this Option because it'll be a semver-breaking change.

I think you mentioned in Zulip the issue is that LLVM doesn't expose the start of text section by default? Could you give more context about this, and how do you define the custom FDE finder? We might be able to find a solution to use one of the existing FDE finder or get the text base using some other method.

@MasterAwesome
Copy link

MasterAwesome commented Aug 5, 2023 •
edited
Loading

How does the text_base currently get set for a platform that requires this to be non-zero?

Also it makes sense for this to be a usize instead of an Option<usize> due the reason Some(0) and None have the same meaning. Instead if you do use Option<NonZeroUSize> to keep the ABI same due to the guaranteed NPO that might be better. What do you think?

@nbdd0121
Copy link
Owner

nbdd0121 commented Aug 5, 2023

If you try to get textrel base when it's not set, you get a panic (which will get turned into abort), so it's not exactly the same as getting a zero.

However it was pointed out that textrel base isn't actually being used and LLVM libunwind does not support it, so it won't have any observable difference. GCC's unwinder does support textrel and will report 0 if not set, but from some digging it seems that it was only used by IA64 and some other non-mainstream architectures and they are no longer generating it.

@MasterAwesome
Copy link

If you try to get textrel base when it's not set, you get a panic (which will get turned into abort), so it's not exactly the same as getting a zero.

Ah I see; thanks for the explanation. How does it impact whether it's set or not; is it purely during what IP is returned? In which case if it isn't supported anymore is the caller expected to be doing the textrel base offsetting to get the non-reloced address? Are there reasons on why the design choice was made by llvm to not support it?

However it was pointed out that textrel base isn't actually being used and LLVM libunwind does not support it, so it won't have any observable difference. GCC's unwinder does support textrel and will report 0 if not set, but from some digging it seems that it was only used by IA64 and some other non-mainstream architectures and they are no longer generating it.

That makes sense, in that case I can see why Option does make sense here to denote the fact that it can be set or not. What do you think about the Option<NonZeroUsize> idea since gcc's set but 0 means the same thing in this case.

@nbdd0121
Copy link
Owner

nbdd0121 commented Aug 5, 2023

No, Option<NonZeroUsize> doesn't make sense because the textrel base can be set and be zero.

@xobs
cargo: support building as part of libstd
02a29af 
Support using `unwinding` as a backend for the libstd `unwind` crate.
This will enable unwinding support for new targets while allowing us to
continue to use `libunwind` from llvm for supported targets.

Signed-off-by: Sean Cross <[email protected]>
@xobs
find_fde: make text_base optional
caab987 
No platforms currently use relative addressing. Make this parameter
optional in order to support targets where this value is not available.

Signed-off-by: Sean Cross <[email protected]>
@xobs
Copy link
Contributor Author

xobs commented Aug 6, 2023

I've rebased on top of trunk.

I think that Option<usize> is the best option going forward, but I don't know what to do about existing targets. I see two dependencies on crates.io, neither of which uses the custom unwinder.

What if all of the unwinders were changed to Option<usize> and this made a 0.3.0 release? I believe that's allowed under semver, since it's 0.x.y.

@nbdd0121
Copy link
Owner

nbdd0121 commented Aug 6, 2023

I'll take this as part of 0.2.0 release. There're some functions that are not supposed to be safe exposed in unwinding::abi and I'd like to fix these, which also is semver breaking change anyway.

@nbdd0121 nbdd0121 merged commit 2f9e374 into nbdd0121:trunk Aug 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nbdd0121 nbdd0121 nbdd0121 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xobs @nbdd0121 @MasterAwesome